fix(profiles, nav): commit identity atomically, and stop navigation losing entries - #175

shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt (1)

32-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Close noOpClient after the tests run.

HttpClient owns an engine and a coroutine scope. This instance is never closed, so each test instance leaks one client. SiloAuthPluginPinTest in the same module closes its clients. Add an @AfterTest teardown.

♻️ Proposed teardown

     private val noOpClient = HttpClient(MockEngine { _ ->
         respond(content = "{}", status = HttpStatusCode.OK, headers = headersOf("Content-Type", "application/json"))
     })
+
+    `@AfterTest`
+    fun tearDown() {
+        noOpClient.close()
+    }

Add the import:

 import kotlin.test.Test
+import kotlin.test.AfterTest

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt`
around lines 32 - 34, Add an `@AfterTest` teardown method to
ProfileIdentityCommitTest that closes the noOpClient HttpClient after each test
instance, importing the required teardown annotation and preserving the existing
client setup.

androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt (2)

52-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move gridScope so the @param clearError KDoc stays on loadProfiles.

The KDoc at lines 52-56 documents clearError. The new gridScope property was inserted between that block and loadProfiles, so the clearError documentation is now attached to gridScope. gridScope has no clearError parameter, and the tag will not resolve.

♻️ Proposed reordering

+    /**
+     * The identity the displayed grid was fetched under.
+     *
+     * Every selection is qualified with it, protected or not. Passing null for
+     * unprotected picks disabled the guard for exactly the case with no PIN
+     * round trip to re-establish scope — so if another account became active
+     * between the grid being accepted and the commit entering the barrier, one
+     * account's profile id was written into the other's token slot.
+     */
+    private var gridScope: AuthScopeSnapshot? = null
+
     /**
      * `@param` clearError false keeps an existing error banner (e.g. a failed
      * delete's explanation) visible across the follow-up list refresh, which
      * would otherwise silently swallow it.
      */
-    /**
-     * The identity the displayed grid was fetched under.
-     *
-     * Every selection is qualified with it, protected or not. Passing null for
-     * unprotected picks disabled the guard for exactly the case with no PIN
-     * round trip to re-establish scope — so if another account became active
-     * between the grid being accepted and the commit entering the barrier, one
-     * account's profile id was written into the other's token slot.
-     */
-    private var gridScope: AuthScopeSnapshot? = null
-
     fun loadProfiles(clearError: Boolean = true) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt`
around lines 52 - 68, Move the gridScope property and its KDoc before the
clearError KDoc, keeping the clearError documentation immediately above
loadProfiles. Ensure gridScope retains its identity-scope documentation and
loadProfiles remains the symbol documented by `@param` clearError.

---

271-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ScopeChanged empties the grid but keeps gridScope in both ViewModels. Both files state the invariant that a captured scope must not outlive the grid it was captured for, and both loadProfiles implementations honor it on the identity-mismatch path. The ScopeChanged handlers do not. If the follow-up loadProfiles() fails, the empty grid keeps a scope from an identity the app no longer holds.

• androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt#L271-L289: set gridScope = null before the _uiState.update block in the ScopeChanged branch of selectProfile.
• androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt#L207-L224: set gridScope = null before the _uiState.update block in the ScopeChanged branch of commitSelection.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt`
around lines 271 - 289, Clear gridScope whenever ScopeChanged empties the
profile grid: in
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt
lines 271-289, set gridScope to null before the _uiState.update block in
selectProfile; likewise in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt
lines 207-224, set gridScope to null before the _uiState.update block in
commitSelection.

androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt (1)

253-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider dropping launchSingleTop on the lobby branch.

TvRoute.WatchTogetherLobby.ROUTE carries a {roomId} argument, so launchSingleTop matches the destination node and not the room. If a lobby entry is ever on top when a second room is opened, Navigation reuses that entry and rewrites its arguments. That is the same node-versus-arguments mismatch this PR removes for item detail and playback.

I could not reach this state from the supplied context: TvWatchTogetherLobbyScreen exposes only onNavigateToPlayer and onBack, and the other entry points run while Main or ItemDetail is on top. Treat this as consistency hardening rather than a live defect.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt`
around lines 253 - 255, Remove launchSingleTop from the lobby navigation branch
in the surrounding navigation logic, and invoke navigate(destination) normally
for TvRoute.WatchTogetherLobby.ROUTE so each roomId creates a distinct lobby
entry.

androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt (1)

40-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Both assertions match the current source; consider a behavioral test for the routing decision.

TvAppNavigation.kt contains navigateToTvWatchTogether(snapshot, lastPlaybackNavigation) at line 905 and tvWatchTogetherDestination(room) at line 245, so these pass.

The assertions bind to identifier names inside the production source. Renaming the lambda parameter snapshot, or the navigateToTvWatchTogether parameter room, fails the test without any behavior change. tvWatchTogetherDestination is a pure function of RoomSnapshot, so a direct assertion on its returned route for a lobby snapshot and a player snapshot would pin the same contract without depending on source text. This matches the pattern the new TvItemDetailNavigationTest uses.

The rest of this class already uses source-text assertions, so this is a pre-existing pattern rather than something the PR introduces.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt`
around lines 40 - 45, Replace the identifier-based source-text assertions in
TvWatchTogetherSurfaceSourceTest with behavioral coverage of the routing
decision: directly verify that tvWatchTogetherDestination returns the expected
route for both lobby and player RoomSnapshot inputs, following the approach used
by TvItemDetailNavigationTest. Keep coverage focused on the destination contract
rather than parameter or implementation names.

androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt (1)

66-68: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Add explicit coverage for percent in TV route arguments.

Encoding contentId, roomId, and title with URLEncoder is appropriate for these route segments, but add a TV route test round-tripping a literal % as %25. The TvAppNavigation matchers already compare decoded arguments, and Navigation Compose 2.9.8 decodes route arguments, so there is no code change needed if that coverage is added.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt`
around lines 66 - 68, Add a TV navigation route test covering a literal percent
sign in the encoded contentId, roomId, and title arguments, asserting the route
contains %25 and the TvAppNavigation matchers receive the original decoded
values. Reuse the existing route-building and matcher test patterns; no
production code changes are needed.

androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt (1)

42-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Two builders produce the pair_device route with different encoders. Route.PairDevice encodes token, code, and serverOrigin with Uri.encode, which leaves : and / unescaped and returns null under plain JVM unit tests. buildPairDeviceRoute encodes the same three arguments with a URLEncoder-based helper. Both routes reach the same destination and the same deviceLoginServerMatch comparison, so the two encodings must agree.

• androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt#L42-L64: replace the three Uri.encode(it) calls with the file's new it.routeEncode() helper.
• androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt#L139-L153: keep the routeEncode encoding, and reuse Route.PairDevice(token, code, serverOrigin).route once both builders share one encoder, so the route exists in one place.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt`
around lines 42 - 64, The two pair-device route builders use inconsistent
encoders. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt#L42-L64,
replace the three Uri.encode calls in Route.PairDevice with the file’s
routeEncode helper. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt#L139-L153,
retain routeEncode and reuse Route.PairDevice(token, code, serverOrigin).route
instead of constructing the route separately.

androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt (1)

55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the blank profileId case.

This test covers a blank serverId and a blank route. It does not cover a blank profileId. That guard is the third of the three checks that stop a wildcard identity scope, so pin it as well.

💚 Proposed addition

         assertNull(
             notificationExternalRouteOrNull(route = "  ", serverId = "server-a", profileId = "kids"),
         )
+        assertNull(
+            notificationExternalRouteOrNull(route = "item/abc", serverId = "server-a", profileId = " "),
+        )
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt`
around lines 55 - 63, Add a third assertion to the `blank is not an identity`
test for `notificationExternalRouteOrNull`, using a valid route and serverId
with a whitespace-only profileId, and assert that it returns null. Keep the
existing blank serverId and route cases unchanged.

androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt (1)

118-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared route-segment decode into one helper.

isSameItemDetail (lines 118-125) and playerRouteIntentOrNull (line 147) use the identical decode expression, including the + preservation step. Both values feed destination-identity comparisons. If one copy changes, the item path and the player path compare differently. Extract a single private helper and call it from both.

♻️ Proposed refactor

+private fun String.decodeRouteSegmentOrNull(): String? =
+    runCatching {
+        URLDecoder.decode(replace("+", "%2B"), StandardCharsets.UTF_8.name())
+    }.getOrNull()?.takeIf(String::isNotBlank)

     val targetContentId = targetRoute
         .substringAfter("item/")
         .substringBefore('?')
         .takeIf { it.isNotBlank() }
         // Same decode as the player intent: the route percent-encodes the id,
         // and an encoded id never equals the decoded one held by the entry.
-        ?.let {
-            runCatching {
-                URLDecoder.decode(it.replace("+", "%2B"), StandardCharsets.UTF_8.name())
-            }.getOrNull()
-        }
-        ?.takeIf { it.isNotBlank() }
+        ?.decodeRouteSegmentOrNull()
         ?: return false

Apply the same call at line 147 in playerRouteIntentOrNull.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt`
around lines 118 - 124, Extract the shared percent-encoded route-segment
decoding, including the plus-sign preservation and failure handling, into a
single private helper in ExternalRouteNavigation. Replace the inline decode in
isSameItemDetail and the identical expression in playerRouteIntentOrNull with
calls to that helper.

androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt (1)

414-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the null live generation case.

This test covers a pinned generation against a known live generation. It does not cover a pinned generation against a null live generation. That case occurs in AppNavigation.kt when tokenManager.snapshotCurrentScope() returns null, and matches then refuses the route. Add the assertion so the fail-closed direction stays pinned.

💚 Proposed addition

         assertFalse(
             scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = 8L),
         )
+        // No live scope at all must not satisfy a generation-pinned route.
+        assertFalse(
+            scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = null),
+        )
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt`
around lines 414 - 428, The test
aScopePinnedToAGenerationDoesNotMatchALaterSession should also assert that a
pinned Identity scope does not match when the live identityGeneration is null,
preserving the fail-closed behavior used by AppNavigation when
snapshotCurrentScope returns null. Add the null-generation matches assertion
alongside the existing generation checks.

androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt (1)

72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use kotlin.test.assertNotEquals instead of the local helper.

The test uses kotlin 2.1.20 and already imports sibling assertions from kotlin.test; add import kotlin.test.assertNotEquals, remove the private helper, and keep the call at line 29 as assertNotEquals("item/abc?seasonNumber=9", route) for String?.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt`
around lines 72 - 76, In ContentDeepLinkEncodingTest, import
kotlin.test.assertNotEquals and remove the local private assertNotEquals helper.
Keep the existing assertion call unchanged so it uses the Kotlin test assertion
with the nullable String route value.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes